Fix Windows FS interop by replacing Unix shell commands with cross‑platform Node.js FS API#55
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows filesystem interoperability by replacing Unix-specific shell commands (test, rm -rf, ls, bash -c) with cross-platform Node.js filesystem APIs. The changes ensure the backend can operate on Windows without relying on Unix commands that produce invalid mixed paths.
Changes:
- Added three new cross-platform helper functions in
file-operations.ts:directoryExists,removeDirectory, andlistDirectoryNames - Replaced all Unix shell command invocations in
repo.tswith the new helper functions - Removed all
bash -cwrapper commands
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| backend/src/services/file-operations.ts | Added three new helper functions for directory operations using Node.js fs promises API |
| backend/src/services/repo.ts | Replaced Unix shell commands with cross-platform helper functions across all repository operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function directoryExists(dirPath: string): Promise<boolean> { | ||
| try { | ||
| const fullPath = path.isAbsolute(dirPath) ? dirPath : path.join(getReposPath(), dirPath) | ||
| const stats = await fs.stat(fullPath) | ||
| return stats.isDirectory() | ||
| } catch { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
The directoryExists function's automatic path resolution behavior (prepending getReposPath() for relative paths) is inconsistent with how callers use it. All callers in repo.ts already construct absolute paths using path.join(getReposPath(), ...) before passing them to this function, making the internal path resolution redundant. This could lead to confusion or bugs if a caller passes a relative path expecting different behavior. Consider either: (1) removing the automatic path resolution and requiring absolute paths, or (2) documenting this behavior clearly and updating callers to pass relative paths directly.
| export async function removeDirectory(dirPath: string): Promise<void> { | ||
| try { | ||
| const fullPath = path.isAbsolute(dirPath) ? dirPath : path.join(getReposPath(), dirPath) | ||
| await fs.rm(fullPath, { recursive: true, force: true }) | ||
| } catch (error) { | ||
| throw new Error(`Failed to remove directory ${dirPath}: ${error}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same path resolution issue as directoryExists. All callers construct absolute paths before calling this function (e.g., removeDirectory(path.join(getReposPath(), dirName))), making the internal path resolution logic redundant and potentially confusing. Consider standardizing the approach across all file operation functions.
| localBranchExists = 'missing' | ||
| } | ||
|
|
||
| if (localBranchExists === 'missing') { |
There was a problem hiding this comment.
The changes from string-based existence checks ('exists'/'missing') to boolean checks are correct, but the logic at lines 399-411 for handling local branch creation when a remote branch is not found lacks test coverage. This is a critical code path for branch creation fallback that should be tested to ensure the Windows compatibility fix works correctly in this scenario.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
The diff introduces a behavior change in cloneRepo (repo.ts) that goes beyond the stated mechanical refactor: the new if (error.message.includes('Remote branch') || error.message.includes('not found')) guard means that for clone failures not matching those strings, the fallback clone is silently skipped — whereas the original code always proceeded to clone after a successful cleanup. This could cause subtle regressions unrelated to Windows FS interop and should be called out explicitly in the PR description or extracted into a separate commit.
Additionally, the verifyRemoved boolean variable is slightly misleading — it holds true when the directory was removed and the subsequent if (!verifyRemoved) checks for failure, but readers have to mentally invert the name to understand it. Renaming it to something like isRemoved or removalConfirmed would make the intent immediately clear.
On the positive side, the internal path resolution logic in removeDirectory and directoryExists (handling both absolute and relative paths via path.isAbsolute) is consistent with how the existing listDirectory function works, and the callers in repo.ts correctly pass already-resolved absolute paths, so there's no double-join issue.
Problem
On Windows, repo operations failed because the backend invoked Unix commands like
test,rm -rf,ls, andbash -c, which produced invalid mixed paths (ie./workspace/repos/C:\Users\...).Fix
bash -cwrappersTesting